Was this a leaked note? It doesn't seem like BIO_f_md has a puts method or am I overlooking something

hm, should be "md_write and md_read both contribute to the digest, md_gets returns digest contents"

NP: This would be easier to read if each field was on a separate line.

I agree, but this was clang-format's doing. I've been using it for formatting stuff in the pkcs7/ module. If there's some set of flags/options that we prefer to use I'm happy to specify them.

NP: In other places, we have a comment next to each value indicating the field name it corresponds to:

static const BIO_METHOD methods_md = {
    BIO_TYPE_MD,       // type
    "message digest",  // name
    md_write,          // bwrite
    md_read,           // bread
    NULL,              // bputs
    md_gets,           // bgets
    md_ctrl,           // ctrl
    md_new,            // create
    md_free,           // destroy
    NULL,              // callback_ctrl
};

This seems to support the use-case of leaving this BIO uninitialized while still reading data from next. In this case, data is returned that has not been digested. Is this use-case tested as well?

hm, good point. this use-case does appear to be supported as far back as OpenSSL 1.1.1. I didn't consider this, I can add a test case.

I do wonder -- should we support this? For compatibility, I'm leaning towards "yes" but it seems like a sharp edge that could cause subtle issues with digest calculation (see also response below)...

OpenSSL's own documentation indicates that the caller should re-initialize after DigestFinal (although it doesn't say anything about initial initialization):

After the digest has been retrieved from a digest BIO it must be reinitialized by calling BIO_reset(), or BIO_set_md() before any more data is passed through it.

Per discussion here, we now require the BIO to be initialized before it can do any IO.

hm, at any rate, it looks like IO on uninitialized BIO's is prevented higher in the call stack in bio.c.

If b was not initialized, should we still be calling BIO_clear_retry_flags and BIO_copy_next_retry?

I think the answer to this is "yes". It would be needed to support its use prior to (or without) initialization.

this is addressed by requiring b to be initialized before it can do any IO.

If this ever returns an error, should we somehow mark this BIO and being not OK for subsequent use? In this case, next has provided new data that can not be reprocessed on any subsequent call.

OpenSSL doesn't do this, but i agree philosophically that we should fail loudly and not silently corrupt the digest.

EVP_DigestUpdate 's documentation stipulates an interface contract of "It returns one". Looking at the implementation, the only case where it can break that contract is if an update function pointer isn't configured on ctx. I believe we can trust that to always be populated as long as ctx has been initialized by EVP_DigestInit_ex.

So, in other words, it looks like EVP_DigestUpdate can only fail if ctx hasn't been initialized. This makes me consider... should we fail b's reads/writes before next's IO if b hasn't been initialized? this would address your concern here and make it harder to misuse the BIO_f_md.

NP: In other places, we have a comment next to each value indicating the field name it corresponds to:

static const BIO_METHOD methods_md = {
    BIO_TYPE_MD,       // type
    "message digest",  // name
    md_write,          // bwrite
    md_read,           // bread
    NULL,              // bputs
    md_gets,           // bgets
    md_ctrl,           // ctrl
    md_new,            // create
    md_free,           // destroy
    NULL,              // callback_ctrl
};

NP: Since this is a non-recoverable error, shouldn't the return value be negative? The BIO_write documentation seems to say that.

// returns the value from calling the |callback_ex|, otherwise |BIO_write|
// returns the number of bytes written, or a negative number on error.

I see OpenSSL returns 0 for this, so compatibility might dictate we do the same. (I'm also bothered that the we're unable to inform the caller about the actual number of bytes written to next b/c we can't return a value indicating success. But that's just the nature of this API.)

As discussed elswhere, EVP_DigestUpdate can only fail if ctx is uninitialized here, but greed that we should return -1 and be consistent with md_read above.

Nit: Not really a priority since it's an internal header, but doing a direct function call instead of macros might be better. Similar to what

Macros allow for potential type mismatches when passing parameters.

hm, that's a good point. i'll adjust this in my next PR.